Fix NaN bugs in the distributed calculation of mean and variance - #2507
Fix NaN bugs in the distributed calculation of mean and variance#2507maliesen wants to merge 18 commits into
Conversation
Co-authored-by: Thomas Saupe <39156931+brownbaerchen@users.noreply.github.com>
Co-authored-by: Thomas Saupe <39156931+brownbaerchen@users.noreply.github.com>
for more information, see https://pre-commit.ci
On one single element the corrected variance should be NaN
If the total number of input data is composed of one single element, the corrected variance should be NaN
Make the internal merge_moments function *not* divide by n - ddof at all and only divide once at the end.
|
|
brownbaerchen
left a comment
brownbaerchen
left a comment
There was a problem hiding this comment.
Looks really good, thanks!
The problem I have is that I am not an expert on this code and that the tests for var are a bit too confusing for me to just accept that surely everything works. That's not your fault, of course.
Before merging this, I would like to replace the tests that make sure the variance of ones is zero with tests that var on random data is the same between heat and numpy. In particular, with all this correction stuff which is very confusing.
If you have time to add that, that would be lovely. Otherwise, I can also do that in a separate PR and then we can merge this once all those tests pass on this branch.
| ) | ||
|
|
||
| else: # case for full matrix calculation (axis is None) | ||
| n = x.lnumel |
There was a problem hiding this comment.
| n = x.lnumel | |
| n = x.larray.numel |
I would like to deprecate the lnumel function at some point because I think it's cleaner to operate on the local array directly when doing anything local.
2 participants
Due Diligence
Description
Issue/s resolved: There was a bug producing nan if the first two ranks are empty as described in #2495. Also, there is bug leading to nan if there is one rank with only one single element if one calls
ht.var(..., axis=0, ddof=1).Changes proposed:
__merge_momentsnot produce anynans by not dividing by (n-ddof) and only divide by n if its safe (which is guaranteed by the early return if n2 == 0)[1] J. Bennett, R. Grout, P. Pebay, D. Roe, D. Thompson, Numerically stable, single-pass, parallel statistics algorithms, IEEE International Conference on Cluster Computing and Workshops, 2009, Oct 2009, New Orleans, LA, USA.
Type of change
Bug fix (non-breaking change which fixes an issue)
Memory requirements
Performance
Does this change modify the behaviour of other functions? If so, which?
yes / no